VRF API refactor #137

tessico · 2022-11-17T12:25:18Z

Description

closes: #125

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (main)
Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
Wrote unit tests
Updated relevant documentation in the code
Added a relevant changelog entry to the Pending section in CHANGELOG.md
Re-reviewed Files changed in the GitHub PR explorer

CLAassistant · 2022-11-17T12:37:38Z

All committers have signed the CLA.

chancharles92 · 2022-11-17T19:13:36Z

@tessico Let me know when the PR is ready, and you can add me and @alxiong as reviewers.

alxiong

Good job 👍 ! @tessico , only a few comments

maybe you can just start a branch from this repo instead of from your fork, (I believe you have write access, right?) so that it's slightly easier for us to switch to your branch without adding another git remote. (thanks!)
please also take a look at the pain of adding trait bound in the downstream here, we should add these traits to our trait VrfScheme::PublicKey/SecretKey/Proof etc) cc @DieracDelta

primitives/src/vrf/blsvrf.rs

alxiong · 2022-11-18T11:27:03Z

primitives/src/vrf/mod.rs

+        &self,
        pp: &Self::PublicParameter,
        proof: &Self::Proof,
        public_key: &Self::PublicKey,
        input: &Self::Input,
    ) -> Result<bool, PrimitivesError>;


@philippecamacho suggested previously that we should add output: Self::Output to the input param to verify() so that the users won't accidentally use the wrong (proof, output) combo.

as you could potentially use a proof, then H_1(proof, "dom sep: app1") and H_2(proof, "dom sep: app2) to get different VRF output.

I believe we knew it's not the most standard-compliant API but it's less error-prone.

cc @fkrell @chancharles92

Do you mean that the verify function will take proof, output, ... and internally check evaluate(proof) =? output as an extra safety check?

I'm against that. It would mean that a user (potentially) runs the evaluate sub-routing twice - first to actually get the VRF output and second to verify that a proof is valid.

Unless the only scenario when someone calls verify(proof, output, ...) is when they are provided both proof and output by a prover - then the "safe" API would make sense.
But I can imagine that a more likely scenario is when the prover produces a proof, runs evaluate for themselves, and only posts/shares a proof without the output - leaving it for the verifier to run evaluate for themselves (once!) and then finally verify(proof, ...).

I think we can have 2 verify APIs, the new set of APIs is as follows:

prove(pp, sec_key, input) -> proof

proof_to_hash(pp, proof) -> output

evaluate(pp, sec_key, input) -> output (i.e. proof_to_hash(pp, prove(pp, sec_key, input)))

verify(pp, pub_key, proof, input) -> (bit, output) (this API also outputs hash as the hash is much cheaper than sig verification and the user doesn't need to call proof_to_hash again to obtain the hash)

verify_hash_consistency(pp, proof, output) -> bit (note that this API is also cheap as it doesn't check signature)

The APIs 1..4 are exactly identical to the standard (https://datatracker.ietf.org/doc/draft-irtf-cfrg-vrf/), the API 5 is used to address the issue mentioned above where the prover and verifier may use an inconsistent hashing scheme so that their outputs are inconsistent.

Actually now I prefer to remove API 5 as the work can be done in API 4: so we just modify the API 4 as follows:
verify(pp, pub_key, proof, input, output') -> (bit, output)
Since API 4 anyway computes output itself, it can add an additional equality check that output' ?= output, which is almost for free.
cc @alxiong @tessico

I agree that we should return (bit, output) from the verify() function. I missed that, sorry.

I would argue, however, that there might be cases where output is not known beforehand and cannot be used as input to the verify routine (unless the user explicitly calls proof_to_hash first). Please correct me if that's never the case.

If it's so, we would have these alternatives:

We give output' an Option<Self::Output> type, i.e. verify(pp, pub_key, proof, input, option(output')) -> (bit, output). If available, we perform the check, and otherwise just return the result.

We delegate correctness checking to the user. This option is fully IRTF draft compatible.

I've got an example of alternative 2 here. The nice thing is that Rust can help with ensuring that output is used, e.g. by the #[must_use] attribute, as shown in the v2 branch. This is my preferred approach. There are numerous ways that users can screw up the protocol, I'm not sure whether we should try to prevent against such fundamental mistakes as using a wrong hash function.

Also as @alxiong suggested I've just created a new branch instead of a branch in a fork. For the sake of keeping these discussions I guess we can keep this PR as is now? There's a branch with the same name and I intend to keep them in-sync if people wanna check it out directly.

If we use alternative 2 (over alternative 1), I'm not fully sure how the user would be able to check that it's in sync with the prover.

If the prover shares only proof, and doesn't provide the output (e.g. only proof is posted on-chain, or some other way), then the verifier has no way to ensure sync anyway.

If the prover shares (proof, output'), then the verifier can call verify(proof, ...) = (bit, output) and for themselves check if output' == output.

@chancharles92

Let's still have evaluate API but provide a default implementation by composing prove and proof_to_hash

So currently I just renamed evaluate -> proof_to_hash.

You saying that we add an extra evaluate(secret_key, input) function corresponding to VRF_hash in the draft as a convenience, right? Would you add that for the Vrf trait or specifically for the BLS VRF as a struct method?

You saying that we add an extra evaluate(secret_key, input) function corresponding to VRF_hash in the draft as a convenience, right? Would you add that for the Vrf trait or specifically for the BLS VRF as a struct method?
Yes, and add it to Vrf trait

@chancharles92 Done: 8513626

I'm a little confused, is that commit on this PR or on another branch @tessico ?

chancharles92 · 2022-11-18T13:13:32Z

primitives/src/vrf/blsvrf.rs

-        let message_bad = [1u8; 32];
-        sign_and_verify::<BLSVRFScheme, Sha256>(&message);
-        failed_verification::<BLSVRFScheme, Sha256>(&message, &message_bad);
+        let message = vec![0u8; 32];


Should we also test the correctness of the hash output somewhere?

Good point, will add

Done in b9f3c50.

chancharles92 · 2022-11-18T13:33:51Z

primitives/src/vrf/mod.rs

+        &self,
        pp: &Self::PublicParameter,
        proof: &Self::Proof,
        public_key: &Self::PublicKey,
        input: &Self::Input,
    ) -> Result<bool, PrimitivesError>;


I think we can have 2 verify APIs, the new set of APIs is as follows:

prove(pp, sec_key, input) -> proof

proof_to_hash(pp, proof) -> output

evaluate(pp, sec_key, input) -> output (i.e. proof_to_hash(pp, prove(pp, sec_key, input)))

verify(pp, pub_key, proof, input) -> (bit, output) (this API also outputs hash as the hash is much cheaper than sig verification and the user doesn't need to call proof_to_hash again to obtain the hash)

verify_hash_consistency(pp, proof, output) -> bit (note that this API is also cheap as it doesn't check signature)

The APIs 1..4 are exactly identical to the standard (https://datatracker.ietf.org/doc/draft-irtf-cfrg-vrf/), the API 5 is used to address the issue mentioned above where the prover and verifier may use an inconsistent hashing scheme so that their outputs are inconsistent.

Composing prove and proof_to_hash calls

- test the hash output directly - test evaluation(sk, m) = proof_to_hash(prove(sk, m)) - generate random test vectors

tessico · 2022-11-21T18:20:11Z

I've updated the branches, and also references in the discussion threads here, such that that all links point to commits in this branch.
This branch tessico/vrf-refactor is in sync with EspressoSys/vrf-refactor branch for convenience of checking it out locally @alxiong.

alxiong

LGTM! (feel free to add trait bounds of associate types on trait Vrf and trait SignatureScheme in a separate PR)

tessico added 4 commits November 17, 2022 13:10

Make BLS VRF input & output a Vector for more flexible hashing

566a30a

dyn hasher instance for BLS VRF

a057fd9

Make all Vrf associated functions into methods

6dcedc2

Update doc comments and enum names

59eb316

reorder imports

1b77b66

tessico requested review from alxiong and chancharles92 November 17, 2022 20:05

alxiong reviewed Nov 18, 2022

View reviewed changes

alxiong requested a review from nyospe November 18, 2022 11:31

chancharles92 reviewed Nov 18, 2022

View reviewed changes

tessico added 3 commits November 18, 2022 19:00

Make output of verify a tuple of (bool, Option<Output>)

f65afd1

Add an evaluate trait method with default implementation

8513626

Composing prove and proof_to_hash calls

more complete tests for VRF

b9f3c50

- test the hash output directly - test evaluation(sk, m) = proof_to_hash(prove(sk, m)) - generate random test vectors

tessico requested review from alxiong, mrain and chancharles92 November 21, 2022 18:22

alxiong approved these changes Nov 22, 2022

View reviewed changes

Merge branch 'main' into vrf-refactor

60069d3

tessico merged commit 668661f into EspressoSystems:main Nov 22, 2022

tessico deleted the vrf-refactor branch November 22, 2022 17:56

tessico mentioned this pull request Nov 23, 2022

Add trait bounds to types in Signature trait #145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VRF API refactor #137

VRF API refactor #137

tessico commented Nov 17, 2022

CLAassistant commented Nov 17, 2022 •

edited

Loading

chancharles92 commented Nov 17, 2022

alxiong left a comment •

edited

Loading

alxiong Nov 18, 2022

tessico Nov 18, 2022

chancharles92 Nov 18, 2022 •

edited

Loading

chancharles92 Nov 18, 2022 •

edited

Loading

tessico Nov 18, 2022 •

edited

Loading

tessico Nov 18, 2022 •

edited

Loading

tessico Nov 18, 2022

chancharles92 Nov 18, 2022

tessico Nov 21, 2022 •

edited

Loading

alxiong Nov 21, 2022

chancharles92 Nov 18, 2022

tessico Nov 18, 2022

tessico Nov 21, 2022

chancharles92 Nov 18, 2022 •

edited

Loading

tessico commented Nov 21, 2022

alxiong left a comment

VRF API refactor #137

VRF API refactor #137

Conversation

tessico commented Nov 17, 2022

Description

CLAassistant commented Nov 17, 2022 • edited Loading

chancharles92 commented Nov 17, 2022

alxiong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chancharles92 Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

chancharles92 Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

tessico Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

tessico Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tessico Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chancharles92 Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

tessico commented Nov 21, 2022

alxiong left a comment

Choose a reason for hiding this comment

CLAassistant commented Nov 17, 2022 •

edited

Loading

alxiong left a comment •

edited

Loading

chancharles92 Nov 18, 2022 •

edited

Loading

chancharles92 Nov 18, 2022 •

edited

Loading

tessico Nov 18, 2022 •

edited

Loading

tessico Nov 18, 2022 •

edited

Loading

tessico Nov 21, 2022 •

edited

Loading

chancharles92 Nov 18, 2022 •

edited

Loading